feat(#243): Adds the rank question type #288
Conversation
🦋 Changeset detectedLatest commit: 1232cd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @eyelidlessness, if you have time, I would really appreciate an early review while I'm working on the test coverage. I'm especially interested in your feedback on the xform-engine implementation. I manually tested |
There was a problem hiding this comment.
I think I've looked at almost all of the engine changes, but I'm going to have to come back to the codecs.
I know there's already a LOT of commentary. I'm not sure how best to provide all the pertinent context without it being overwhelming, so let me know if you'd find it more helpful to jump on a call or something!
Either way, I wanted to share my thoughts so far before my brain runs out of brain juice for the day.
packages/xforms-engine/src/parse/body/control/RankControlDefinition.ts
Outdated
Show resolved
Hide resolved
packages/xforms-engine/src/parse/body/control/RankControlDefinition.ts
Outdated
Show resolved
Hide resolved
packages/xforms-engine/src/parse/body/control/RankControlDefinition.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I know there are some changes incoming, particularly on the UI side, so I took a light touch there for now.
And I think I've covered everything that I could on the engine side.
One thing that we'll still need to address is @getodk/scenario. At minimum, it'll need to pass type checks. Mostly that will involve creating and referencing rank-specific equivalents of various things (a good starting point would be to reference select-specific things in scenario package). I think I also mentioned a few specific test cases that would make sense to add. I'm happy to help, I know there's a whole slew of other new concepts to discover there!
@latin-panda All in all, this is an excellent start! I am honestly more than a little mind boggled by how much context you managed to gain on your own with very little information or guidance. I hope the feedback so far is helpful getting this closer to the finish line, and I'm looking forward to taking another pass when you're ready!
packages/xforms-engine/src/lib/codecs/BaseItemCollectionCodec.ts
Outdated
Show resolved
Hide resolved
packages/xforms-engine/src/lib/reactivity/createItemCollection.ts
Outdated
Show resolved
Hide resolved
packages/xforms-engine/test/lib/reactivity/createSelectItems.test.ts
Outdated
Show resolved
Hide resolved
…t and last element
|
I've attached videos to the PR description about some of the tests I did. Here: |
Simplify scenario API for rank and use naming convention
There was a problem hiding this comment.
I think this wraps up my feedback. Great work @latin-panda! 🎉
I think the only things outstanding are the fix for hasAllValues, and ensuring the test exercising it gets updated to account for the error it'll throw. Otherwise LGTM! I'm excited to get this in.
| } | ||
|
|
||
| if (highlight.index.value !== null) { | ||
| highlight.timeoutID = setTimeout(() => setHighlight(null), 1000); |
There was a problem hiding this comment.
No need for any action on this here, just noting a couple things I observed/thought about:
- This delay feels longer than I'd expect. And I think tweaking it might be aided by making it a
SHOUTY_CONSTANTlikeHOLD_DELAYis. - There's a pixel shift while the highlight is applied, because the border width changes. I expect we'll make other presentation changes, so I'm not too concerned about it for now.
There was a problem hiding this comment.
This delay feels longer than I'd expect. And I think tweaking it might be aided by making it a SHOUTY_CONSTANT like HOLD_DELAY is.
I've added constant with JS Docs, and made it 500ms
There's a pixel shift while the highlight is applied, because the border width changes. I expect we'll make other presentation changes, so I'm not too concerned about it for now.
With the new Figma style, this is not a problem. It's temporary. For now, it looks like the highlight from select, which is a border of 2px
|
|
||
| interface HighlightOption { | ||
| index: Ref<number | null>; | ||
| timeoutID: NodeJS.Timeout | null; |
There was a problem hiding this comment.
Definitely something to solve separately, but this is a really good example of how the TypeScript config is broken for Vue. The type for a web environment should be number | null, and we shouldn't have Node types in scope here at all. Just making note because I've also run into issues with this.
There was a problem hiding this comment.
Yes! Agree 100%. I tried other types, but I was still getting the errors
There was a problem hiding this comment.
Yeah. The config and tooling around Vue is kind of a mess. I've come back to it a few times, but always end up frustrated. Last time I looked, there were a lot of improvements in flight on the tooling front, so maybe I'll take another look at it again soon.
| @media screen and (max-width: #{$sm}) { | ||
| .rank-buttons { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure about this! The most likely way a user would experience it is on a phone. I think the buttons are especially convenient for a touch interface (where drag/drop can be really awkward, even with finesse like an appropriate delay). I am happy to defer this for now, since it'll probably need more UI/UX design attention. But wanted to note that it feels like a gap to me.
There was a problem hiding this comment.
The reason @alyblenkin and I removed the buttons on mobile devices was to create more space for text and reduce excessive wrapping, which makes reading harder. Buttons take up too much screen space.
There was a problem hiding this comment.
Yeah, that's what I assumed. Like I said, I'm happy to defer for now. But I don't think it's the best UX compromise. The place they're hidden is the place I'd want them most. I don't know if that's representative of more users, and that's a good enough reason to me for coming back to it another time! 🚀
|
Just did a quick manual test on my iPhone. Nothing concerning on Safari (iOS)! |
… delay in component
There was a problem hiding this comment.
🎉 Awesome awesome work @latin-panda! Feel free to merge once CI passes 🚀
|
Thank you for reviewing @eyelidlessness |
Closes #243
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
random,choice-filter,relevantandrequiredWhy is this the best possible solution? Were any other approaches considered?
This implementation takes
selectas reference and reuses some of its code.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
Please assist in testing in iOS because I don't have an iPhone or iPad. Other than that, I did the following tests:
Test scenario video: Readonly and translation
The rank question has a condition where it becomes readonly when there are less than 5 options selected. It also gets translated into French.
2-rank-with-choice-filter-read-only.xlsx
readonly-and-translations.mp4
Test scenario video: It should work in all browsers we support. Also, rank as required question.
It works in Chrome, Firefox, Safari and Chrome for Android.
It shows rank working as required questions.
Browser.testing.-.Chrome.Fireforx.Safari.mp4
chrome.for.android.mp4
Test scenario video: Responsive design and becoming relevant again has value assigned
It shows rank in responsive design, where the buttons are hidden for phone devices.
It also tests when the user ordered a rank's options, then it became no-relevant, and relevant again. At this point, the rank has a value assigned, and the user can submit the form without ordering the options again.
responsive.design.and.becoming.relevant.again.has.value.set.mp4
What's changed
selectandrank